fix: Persist settings panel preferences to localStorage#32
fix: Persist settings panel preferences to localStorage#32
Conversation
Connect SettingsPanel UI controls to useSettingsStore hook so that Weekly Hours Target and Timesheet Reminders preferences are saved to localStorage and persist across browser sessions. Fixes #10
There was a problem hiding this comment.
Pull request overview
This PR connects the SettingsPanel UI controls to the useSettingsStore hook, enabling persistence of Weekly Hours Target and Timesheet Reminders preferences to localStorage via Zustand's persist middleware. Previously, these settings used defaultValue and defaultChecked attributes which did not persist across sessions.
- Changed Weekly Hours Target input from uncontrolled (
defaultValue) to controlled (value) with store integration - Changed Timesheet Reminders checkbox from uncontrolled (
defaultChecked) to controlled (checked) with store integration - Added
useSettingsStorehook import and integration to manage state persistence
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <input | ||
| type="number" | ||
| defaultValue={40} | ||
| value={weeklyHoursTarget} | ||
| onChange={(e) => | ||
| updateSettings({ | ||
| weeklyHoursTarget: Math.max(0, parseInt(e.target.value) || 0), | ||
| }) | ||
| } | ||
| min={0} | ||
| className="w-20 rounded-lg border border-dark-600 bg-dark-800 px-3 py-2 text-dark-100 focus:outline-none focus:ring-2 focus:ring-thyme-500" | ||
| /> |
There was a problem hiding this comment.
The number input lacks an accessible label. The input should have either an explicit label association via htmlFor/id or an aria-label attribute to ensure screen reader users can understand the purpose of this input field. The visible text "Weekly Hours Target" above the input is not programmatically associated with the input element.
| value={weeklyHoursTarget} | ||
| onChange={(e) => | ||
| updateSettings({ | ||
| weeklyHoursTarget: Math.max(0, parseInt(e.target.value) || 0), |
There was a problem hiding this comment.
When the input is cleared completely (empty string), parseInt will return NaN, and the fallback to 0 will set weeklyHoursTarget to 0. This means users cannot backspace to clear the field and type a new value without the field immediately resetting to 0. Consider using a controlled input pattern that allows temporary invalid states while typing, or use a separate local state to handle the input value and only update the store onBlur.
| value={weeklyHoursTarget} | |
| onChange={(e) => | |
| updateSettings({ | |
| weeklyHoursTarget: Math.max(0, parseInt(e.target.value) || 0), | |
| defaultValue={weeklyHoursTarget} | |
| onBlur={(e) => | |
| updateSettings({ | |
| weeklyHoursTarget: Math.max( | |
| 0, | |
| Number.isNaN(parseInt(e.target.value, 10)) ? 0 : parseInt(e.target.value, 10) | |
| ), |
| type="checkbox" | ||
| checked={notificationsEnabled} | ||
| onChange={(e) => updateSettings({ notificationsEnabled: e.target.checked })} | ||
| className="peer sr-only" |
There was a problem hiding this comment.
The checkbox input lacks an accessible label. While the checkbox has a visible toggle switch, it should have either an aria-label attribute or be wrapped in a label element that includes the text "Timesheet Reminders" to ensure screen reader users can understand what the checkbox controls. The current label element only contains the visual toggle, not the descriptive text.
| className="peer sr-only" | |
| className="peer sr-only" | |
| aria-label="Timesheet Reminders" |
Connect SettingsPanel UI controls to useSettingsStore hook so that
Weekly Hours Target and Timesheet Reminders preferences are saved
to localStorage and persist across browser sessions.
Fixes #10